Open
Conversation
Summary by CodeRabbitRelease NotesBug Fixes
Tests
WalkthroughThe PR introduces a utility function Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@models/discordactions.js`:
- Around line 696-697: The current conditional in the idle-days check (if
(idleDays <= 7) return;) incorrectly excludes users with exactly 7 idle days;
update the boundary to be inclusive of 7 days by changing the condition to check
for strictly less than 7 (i.e., use idleDays < 7) in the function handling the
"group-idle-7d+" logic so users with exactly 7 days are included; locate the
conditional that references idleDays in models/discordactions.js and replace the
comparison accordingly.
- Around line 687-695: The idle-7d scan is doing an extra Firestore read per
user by calling userStatusModel.doc(userStatus.id).get() inside the Promise.all;
remove that read and instead ensure the userStatus objects already include the
fields needed by computeIdleDaysExcludingOOO (idleWindowStartedAt, lastOooFrom,
lastOooUntil, and currentStatus.from). Update the producer that builds
userStatus (the getAllUserStatus function in models/userStatus.js) to populate
those fields on each userStatus so the code in models/discordactions.js can call
computeIdleDaysExcludingOOO using the existing userStatus object without any
further doc.get().
In `@models/userStatus.js`:
- Around line 256-258: When transitioning back to ACTIVE, clear any stale idle
anchor by resetting newStatusData.idleWindowStartedAt to null/undefined;
specifically, in the state-transition logic that checks requestedNextState and
previousState (the branch where requestedNextState === userState.ACTIVE and
previousState === userState.IDLE or similar), set
newStatusData.idleWindowStartedAt = null (or delete it) so the idle-day
calculation doesn't use a stale value; update the same control paths that set
idleWindowStartedAt (the branch using currentStatus?.from ??
currentStatus?.updatedAt) to ensure symmetry.
In `@test/unit/utils/userStatus.test.js`:
- Around line 114-139: The time-based tests calling computeIdleDaysExcludingOOO
use multiple Date.now() calls which can drift; fix each affected "it" block (the
tests "should return total idle days when no OOO period", "should exclude last
OOO period from idle days", and "should fall back to currentStatusFrom when
idleWindowStartedAt is missing") by capturing a single const now = Date.now() at
the top of the test and deriving windowStart, oooFrom, oooUntil, and
currentStatusFrom from that single now before calling
computeIdleDaysExcludingOOO so all timestamps are deterministic.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
models/discordactions.jsmodels/userStatus.jstest/unit/utils/userStatus.test.jsutils/userStatus.js
Comment on lines
+687
to
+695
| const fullStatusDoc = await userStatusModel.doc(userStatus.id).get(); | ||
| const fullData = fullStatusDoc.exists ? fullStatusDoc.data() : {}; | ||
| const idleDays = computeIdleDaysExcludingOOO( | ||
| fullData.idleWindowStartedAt, | ||
| fullData.lastOooFrom, | ||
| fullData.lastOooUntil, | ||
| userStatus.currentStatus?.from, | ||
| nowMs | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid N+1 Firestore reads in idle-7d scan.
Line 687 adds one extra usersStatus read per user inside Promise.all. This can significantly increase read costs and runtime for large idle cohorts.
⚡ Suggested direction
- const fullStatusDoc = await userStatusModel.doc(userStatus.id).get();
- const fullData = fullStatusDoc.exists ? fullStatusDoc.data() : {};
+ // Prefer including these fields directly in getAllUserStatus payload:
+ // idleWindowStartedAt, lastOooFrom, lastOooUntil
+ const fullData = userStatus;And extend the producer payload (in models/userStatus.js getAllUserStatus) so userStatus already contains these fields.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@models/discordactions.js` around lines 687 - 695, The idle-7d scan is doing
an extra Firestore read per user by calling
userStatusModel.doc(userStatus.id).get() inside the Promise.all; remove that
read and instead ensure the userStatus objects already include the fields needed
by computeIdleDaysExcludingOOO (idleWindowStartedAt, lastOooFrom, lastOooUntil,
and currentStatus.from). Update the producer that builds userStatus (the
getAllUserStatus function in models/userStatus.js) to populate those fields on
each userStatus so the code in models/discordactions.js can call
computeIdleDaysExcludingOOO using the existing userStatus object without any
further doc.get().
Comment on lines
+696
to
+697
| if (idleDays <= 7) { | ||
| return; |
There was a problem hiding this comment.
Potential 7d+ boundary mismatch (<= 7 skips exactly 7 days).
At Line 696, users with exactly 7 idle days are excluded. For a group-idle-7d+ rule, boundary is typically inclusive.
✅ Boundary fix
- if (idleDays <= 7) {
+ if (idleDays < 7) {
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (idleDays <= 7) { | |
| return; | |
| if (idleDays < 7) { | |
| return; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@models/discordactions.js` around lines 696 - 697, The current conditional in
the idle-days check (if (idleDays <= 7) return;) incorrectly excludes users with
exactly 7 idle days; update the boundary to be inclusive of 7 days by changing
the condition to check for strictly less than 7 (i.e., use idleDays < 7) in the
function handling the "group-idle-7d+" logic so users with exactly 7 days are
included; locate the conditional that references idleDays in
models/discordactions.js and replace the comparison accordingly.
Comment on lines
+256
to
258
| if (requestedNextState === userState.IDLE && previousState === userState.ACTIVE) { | ||
| newStatusData.idleWindowStartedAt = newStatusData.currentStatus?.from ?? newStatusData.currentStatus?.updatedAt; | ||
| } |
There was a problem hiding this comment.
Reset idleWindowStartedAt when moving to ACTIVE to prevent stale idle anchors.
Line 256 and Line 625 set idleWindowStartedAt for ACTIVE→IDLE, but these paths do not clear it when transitioning back to ACTIVE. That can overcount idle duration later because idle-day calculation prefers this field.
🛠️ Suggested fix
// in updateUserStatus(...)
if (requestedNextState === userState.IDLE && previousState === userState.ACTIVE) {
newStatusData.idleWindowStartedAt = newStatusData.currentStatus?.from ?? newStatusData.currentStatus?.updatedAt;
+} else if (requestedNextState === userState.ACTIVE) {
+ newStatusData.idleWindowStartedAt = null;
}
// in batchUpdateUsersStatus(...)
if (state === userState.IDLE && currentState === userState.ACTIVE) {
updatedStatusData.idleWindowStartedAt = statusToUpdate.from ?? currentTimeStamp;
+} else if (state === userState.ACTIVE) {
+ updatedStatusData.idleWindowStartedAt = null;
}Also applies to: 625-627
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@models/userStatus.js` around lines 256 - 258, When transitioning back to
ACTIVE, clear any stale idle anchor by resetting
newStatusData.idleWindowStartedAt to null/undefined; specifically, in the
state-transition logic that checks requestedNextState and previousState (the
branch where requestedNextState === userState.ACTIVE and previousState ===
userState.IDLE or similar), set newStatusData.idleWindowStartedAt = null (or
delete it) so the idle-day calculation doesn't use a stale value; update the
same control paths that set idleWindowStartedAt (the branch using
currentStatus?.from ?? currentStatus?.updatedAt) to ensure symmetry.
Comment on lines
+114
to
+139
| it("should return total idle days when no OOO period", function () { | ||
| const windowStart = Date.now() - 10 * ONE_DAY_MS; | ||
| const now = Date.now(); | ||
| const days = computeIdleDaysExcludingOOO(windowStart, null, null, null, now); | ||
| expect(days).to.equal(10); | ||
| }); | ||
|
|
||
| it("should exclude last OOO period from idle days", function () { | ||
| const windowStart = Date.now() - 15 * ONE_DAY_MS; | ||
| const oooFrom = Date.now() - 10 * ONE_DAY_MS; | ||
| const oooUntil = Date.now() - 5 * ONE_DAY_MS; | ||
| const now = Date.now(); | ||
| const days = computeIdleDaysExcludingOOO(windowStart, oooFrom, oooUntil, null, now); | ||
| expect(days).to.equal(10); | ||
| }); | ||
|
|
||
| it("should fall back to currentStatusFrom when idleWindowStartedAt is missing", function () { | ||
| const currentStatusFrom = Date.now() - 8 * ONE_DAY_MS; | ||
| const now = Date.now(); | ||
| const days = computeIdleDaysExcludingOOO(null, null, null, currentStatusFrom, now); | ||
| expect(days).to.equal(8); | ||
| }); | ||
|
|
||
| it("should return 0 when window has no span", function () { | ||
| const now = Date.now(); | ||
| const days = computeIdleDaysExcludingOOO(now, null, null, null, now); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make time-based tests deterministic with a single baseline timestamp.
At Line 115, Line 123, and Line 125, each value is derived from separate Date.now() calls. Use one now per test and derive all timestamps from it to avoid timing drift in CI.
♻️ Suggested stabilization
it("should return total idle days when no OOO period", function () {
- const windowStart = Date.now() - 10 * ONE_DAY_MS;
- const now = Date.now();
+ const now = Date.now();
+ const windowStart = now - 10 * ONE_DAY_MS;
const days = computeIdleDaysExcludingOOO(windowStart, null, null, null, now);
expect(days).to.equal(10);
});
it("should exclude last OOO period from idle days", function () {
- const windowStart = Date.now() - 15 * ONE_DAY_MS;
- const oooFrom = Date.now() - 10 * ONE_DAY_MS;
- const oooUntil = Date.now() - 5 * ONE_DAY_MS;
- const now = Date.now();
+ const now = Date.now();
+ const windowStart = now - 15 * ONE_DAY_MS;
+ const oooFrom = now - 10 * ONE_DAY_MS;
+ const oooUntil = now - 5 * ONE_DAY_MS;
const days = computeIdleDaysExcludingOOO(windowStart, oooFrom, oooUntil, null, now);
expect(days).to.equal(10);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unit/utils/userStatus.test.js` around lines 114 - 139, The time-based
tests calling computeIdleDaysExcludingOOO use multiple Date.now() calls which
can drift; fix each affected "it" block (the tests "should return total idle
days when no OOO period", "should exclude last OOO period from idle days", and
"should fall back to currentStatusFrom when idleWindowStartedAt is missing") by
capturing a single const now = Date.now() at the top of the test and deriving
windowStart, oooFrom, oooUntil, and currentStatusFrom from that single now
before calling computeIdleDaysExcludingOOO so all timestamps are deterministic.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Date: 27-02-2026
Developer Name: @vinit717
Issue Ticket Number
Description
Fix the 7 days+ idle users calculation logic
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
Test Coverage
Screenshot 1
Additional Notes